subplot titles respecting row_width and column_width #1245
Conversation
|
@jonmmease ready for a review |
There was a problem hiding this comment.
Thanks for taking this on @Kully! A few comments/questions inline.
Also, do we have any tests that would make sure that this rowspan example from the docs is still working (https://plot.ly/python/subplots/#custom-sized-subplot-with-subplot-titles)?
Thanks!
|
|
||
| def test_row_width_and_column_width(self): | ||
|
|
||
| expected = Figure({ |
There was a problem hiding this comment.
Could you add an image of this expected figure to the PR?
There was a problem hiding this comment.
just a png in the file? I am not aware of image tests for this repo but I could be wrong
There was a problem hiding this comment.
No, I just meant in the GitHub PR so I could see what it looks like 🙂
plotly/tools.py
Outdated
| @@ -1080,6 +1079,7 @@ def _get_anchors(r, c, x_cnt, y_cnt, shared_xaxes, shared_yaxes): | |||
| return x_anchor, y_anchor | |||
|
|
|||
| list_of_domains = [] # added for subplot titles | |||
| list_of_domains_complete = [] # hold onto every domain used, even if shared axes | |||
plotly/tools.py
Outdated
| subtitle_pos_y = [None] * rows | ||
| delt_x = (x_e - x_s) | ||
| x_dom = [layout[k]['domain'] for k in sorted(layout.to_plotly_json().keys()) if 'xaxis' in k] | ||
| y_dom = [layout[k]['domain'] for k in sorted(layout.to_plotly_json().keys()) if 'yaxis' in k] |
There was a problem hiding this comment.
Is this sort going to break down when there are more than 9 xaxes? e.g. ['xaxis', 'xaxis10', 'xaxis2', 'xaxis3', ...]
I'm thinking we might need to pluck off the digits and sort them as integers. Could you try an example with 11 or 12 rows and see if there's an issue? If there is an issue, let's make it a test 🙂
There was a problem hiding this comment.
great catch - I'll make sure it's full proof
| subtitle_pos_x.append(sum(x_domains) / 2) | ||
| subtitle_pos_x *= rows | ||
|
|
||
| if shared_yaxes: |
There was a problem hiding this comment.
Do we have any tests that specify row_width and hit this branch?
|
Code looks good. Last thing, could you add a screenshot (in the PR) and test for this example from the documentation: https://plot.ly/python/subplots/#custom-sized-subplot-with-subplot-titles I want to make sure this doesn't break cases with |
Looks good to me.
|
|
Great! Just make it a test and we're good to go. I'll merge it in tonight. Thanks! |
we already have a test that already tests this example except for the data: https://github.com/plotly/plotly.py/blob/master/plotly/tests/test_core/test_tools/test_make_subplots.py#L2086-L2090 Do you want something that also checks the data or is't it good enough that the layout is being tested? |
|
No, that covers it. Didn't realize we had this one already! 💃 |
* first pass at subplot titles respecting row_width and column_width * add test for row_width and column_width in make_subplots
issue: #1229
Going to write some tests that ensure that subtitles are in the right place w/ row_/column_width, if none exist already.
Still fixing broken tests